feat(m365): add DMARC domain check#11840
Open
0xTaoZ wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
provider/m365
Contributor
|
✅ No Conflicts No conflict markers, and the branch merges cleanly into its base. |
📝 WalkthroughWalkthroughAdds a new Microsoft 365 Defender check, defender_domain_dmarc_records_published, which verifies each Exchange Online domain publishes an enforcing DMARC TXT record via DNS lookup, along with check metadata and unit tests covering pass/fail/edge-case scenarios. ChangesDMARC enforcement check
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Check as defender_domain_dmarc_records_published
participant DefenderClient as Defender client
participant DNS as DNS resolver
Check->>DefenderClient: read dkim_configurations
loop for each domain
Check->>DNS: query TXT _dmarc.<domain>
DNS-->>Check: TXT records or NXDOMAIN
Check->>Check: extract p= policy via regex
Check->>Check: set PASS if quarantine/reject else FAIL
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@prowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.py`:
- Around line 1-7: The module-level imports in
defender_domain_dmarc_records_published are missing an explicit dependency on
dns.exception, even though dns.exception.DNSException is referenced later in the
check logic. Update the imports near the top of
defender_domain_dmarc_records_published.py to explicitly import dns.exception
alongside dns.resolver, so the exception handling in the check remains clear and
robust.
- Around line 62-66: The _get_dmarc_policy helper currently calls
dns.resolver.resolve without a bounded DNS wait, so add explicit timeout
handling there. Update the resolve call for _dmarc.{domain} TXT lookups to pass
a finite lifetime/timeout and keep the existing DNSException fallback so slow or
unresponsive domains do not stall the scan.
In
`@tests/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published_test.py`:
- Around line 139-153: Rename the empty-result test in
defender_domain_dmarc_records_published_test.py from test_no_dkim_configurations
to test_no_resources so it matches the standard naming convention for checks;
keep the existing test body and assertion in test_no_resources, and leave the
PASS/FAIL coverage tests unchanged. Use the current test method definition as
the only symbol to update.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f1d31ac1-e60e-465a-b61c-84322fd444a3
📒 Files selected for processing (4)
prowler/providers/m365/services/defender/defender_domain_dmarc_records_published/__init__.pyprowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.metadata.jsonprowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.pytests/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published_test.py
Comment on lines
+1
to
+7
| import re | ||
| from typing import List | ||
|
|
||
| import dns.resolver | ||
|
|
||
| from prowler.lib.check.models import Check, CheckReportM365 | ||
| from prowler.providers.m365.services.defender import defender_client |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
dns.exception is used but never explicitly imported.
Only dns.resolver is imported; dns.exception.DNSException (line 65) works only because dns.resolver transitively imports dns.exception and registers it as an attribute of the dns package. This is fragile — an explicit import documents the dependency and protects against future dnspython internal changes.
🧹 Proposed fix
import re
from typing import List
+import dns.exception
import dns.resolverAlso applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@prowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.py`
around lines 1 - 7, The module-level imports in
defender_domain_dmarc_records_published are missing an explicit dependency on
dns.exception, even though dns.exception.DNSException is referenced later in the
check logic. Update the imports near the top of
defender_domain_dmarc_records_published.py to explicitly import dns.exception
alongside dns.resolver, so the exception handling in the check remains clear and
robust.
Comment on lines
+62
to
+66
| def _get_dmarc_policy(domain: str) -> str: | ||
| try: | ||
| txt_records = dns.resolver.resolve(f"_dmarc.{domain}", "TXT") | ||
| except dns.exception.DNSException: | ||
| return "" |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant Defender DNS lookup checks
rg -n "dns\.resolver\.resolve|Resolver\(|lifetime=|timeout=" prowler/providers/m365/services/defender -g '*.py'
# Inspect the target file with line numbers
cat -n prowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.py
# Inspect any sibling SPF/DKIM checks if present
fd -a -t f '.*(spf|dkim|dmarc).*\.py$' prowler/providers/m365/services/defenderRepository: prowler-cloud/prowler
Length of output: 3967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n prowler/providers/m365/services/defender/defender_domain_dkim_enabled/defender_domain_dkim_enabled.py
# Look for any explicit dnspython timeout patterns elsewhere in the repo
rg -n "dns\.resolver\.resolve\(|lifetime=|timeout=" prowler -g '*.py'Repository: prowler-cloud/prowler
Length of output: 7492
Add a DNS timeout to dns.resolver.resolve. This call currently relies on dnspython’s default lifetime, so a few slow or unresponsive domains can stall the scan; pass a bounded lifetime/timeout here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@prowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.py`
around lines 62 - 66, The _get_dmarc_policy helper currently calls
dns.resolver.resolve without a bounded DNS wait, so add explicit timeout
handling there. Update the resolve call for _dmarc.{domain} TXT lookups to pass
a finite lifetime/timeout and keep the existing DNSException fallback so slow or
unresponsive domains do not stall the scan.
Comment on lines
+139
to
+153
| def test_no_dkim_configurations(self): | ||
| defender_client = _mock_defender_client() | ||
|
|
||
| with ( | ||
| _mock_m365_provider(), | ||
| _mock_defender_init(), | ||
| mock.patch( | ||
| f"{CHECK_MODULE}.defender_client.defender_client", | ||
| new=defender_client, | ||
| ), | ||
| ): | ||
| check = _get_check_class()() | ||
| result = check.execute() | ||
|
|
||
| assert len(result) == 0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename test_no_dkim_configurations to test_no_resources.
As per path instructions, "Every check needs test_no_resources (len==0), pass (PASS) and fail (FAIL)." This test covers the correct scenario (empty result set) but uses a non-standard name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published_test.py`
around lines 139 - 153, Rename the empty-result test in
defender_domain_dmarc_records_published_test.py from test_no_dkim_configurations
to test_no_resources so it matches the standard naming convention for checks;
keep the existing test body and assertion in test_no_resources, and leave the
PASS/FAIL coverage tests unchanged. Use the current test method definition as
the only symbol to update.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
defender_domain_dmarc_records_publishedfor Microsoft 365 Defender_dmarc.<domain>TXT recordp=quarantineorp=reject)Closes #11800
Validation
uv run pytest tests/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published_test.py -quv run black --check --target-version py310 prowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.py tests/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published_test.pyuv run flake8 prowler/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published.py tests/providers/m365/services/defender/defender_domain_dmarc_records_published/defender_domain_dmarc_records_published_test.pyRisk
Low. The check is additive and only performs DNS TXT lookups for domains already present in the Defender DKIM configuration list. DNS lookup failures are treated as non-enforcing DMARC findings instead of raising.